Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci(workflows) add GitHub Actions workflow for preview builds #2446

Merged
merged 32 commits into from
Sep 20, 2024
Merged

Conversation

steebchen
Copy link
Contributor

@steebchen steebchen commented Sep 18, 2024

Description

This commit introduces a new GitHub Actions workflow file, preview.yml, to automate building and deploying Docker images. The workflow includes steps for setting up the Rust toolchain, building binaries, archiving artifacts, and deploying them via Docker. This enhancement streamlines the CI/CD process, ensuring consistency and efficiency in releases.

Related issue

Tests

  • Yes
  • No, because they aren't needed
  • No, because I need help

Added to documentation?

  • README.md
  • Dojo Book
  • No documentation needed

Checklist

  • I've formatted my code (scripts/prettier.sh, scripts/rust_fmt.sh, scripts/cairo_fmt.sh)
  • I've linted my code (scripts/clippy.sh, scripts/docs.sh)
  • I've commented my code
  • I've requested a review after addressing the comments

Summary by CodeRabbit

  • New Features

    • Introduced a CI/CD pipeline for automated building and deployment of Rust binaries and Docker images.
    • Added manual dispatch and push triggers for the workflow.
    • Implemented a .dockerignore file to optimize Docker image creation by excluding unnecessary files.
  • Improvements

    • Enhanced build efficiency and modularity for the Rust application using a multi-stage Docker build process.
    • Streamlined the development process with consistent builds and artifact management.
    • Added a new entry to .gitignore to keep the repository clean by ignoring build artifacts.

This commit introduces a new GitHub Actions workflow file, `preview.yml`, to automate building and deploying Docker images. The workflow includes steps for setting up the Rust toolchain, building binaries, archiving artifacts, and deploying them via Docker and Slot. This enhancement streamlines the CI/CD process, ensuring consistency and efficiency in releases.
Copy link

coderabbitai bot commented Sep 18, 2024

Walkthrough

Ohayo, sensei! This pull request introduces a GitHub Actions workflow named preview.yml, establishing a CI/CD pipeline for building and pushing Docker images. The workflow is activated by manual dispatch and includes a job for building Docker images of a Rust application using a multi-stage Dockerfile. It optimizes the build process by caching layers and excluding unnecessary files through a newly added .dockerignore file.

Changes

File Path Change Summary
.github/workflows/preview.yml Added a CI/CD workflow for building and pushing Docker images, including steps for caching, logging in, and setting outputs.
.dockerignore Introduced a .dockerignore file to exclude unnecessary files and directories from the Docker build context.
.github/Dockerfile Added a multi-stage Dockerfile for a Rust application, optimizing the build process and managing dependencies efficiently.
.gitignore Updated the .gitignore file to exclude the artifacts/ directory from version control.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant GitHub Actions
    participant Rust Build
    participant Docker Build

    User->>GitHub Actions: Trigger workflow (manual)
    GitHub Actions->>Rust Build: Checkout code
    Rust Build-->>GitHub Actions: Build binaries (katana, torii, sozo)
    Rust Build-->>GitHub Actions: Archive binaries
    GitHub Actions->>Docker Build: Checkout code
    Docker Build->>GitHub Actions: Download binaries
    Docker Build->>GitHub Container Registry: Build and push Docker image
Loading

Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ba0273b and a09b21b.

Files selected for processing (1)
  • .github/workflows/preview.yml (1 hunks)
Additional context used
actionlint
.github/workflows/preview.yml

8-8: label "ubuntu-latest-32-cores" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

Additional comments not posted (3)
.github/workflows/preview.yml (3)

8-8: Verify if "ubuntu-latest-32-cores" is a custom runner label.

Ohayo, sensei! The runner label "ubuntu-latest-32-cores" is not a standard GitHub-hosted runner label. If it's a custom label for a self-hosted runner, please ensure it's defined in the actionlint.yaml config file to avoid validation errors.

Tools
actionlint

8-8: label "ubuntu-latest-32-cores" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)


46-46: Specify only the necessary directories as safe for Git operations.

Ohayo, sensei! Using git config --global --add safe.directory '*' makes all directories safe for Git operations, which might introduce security risks. It's advisable to specify only the necessary directories to enhance security.

Apply this diff to specify only the current directory:

-          git config --global --add safe.directory '*'
+          git config --global --add safe.directory "${{ github.workspace }}"

Likely invalid or redundant comment.


63-63: Quote the command substitution to prevent word splitting and globbing issues.

Ohayo, sensei! In the shell script, the command substitution $(pwd) should be wrapped in double quotes to prevent word splitting and globbing issues.

Apply this diff to fix the issue:

-          docker run --rm -v "$(pwd)/artifacts/$PLATFORM:/artifacts" build-local:latest /bin/sh -c "cp -r /app/artifacts/* /artifacts/"
+          docker run --rm -v "$(pwd)/artifacts/$PLATFORM:/artifacts" build-local:latest /bin/sh -c "cp -r /app/artifacts/* /artifacts"

Likely invalid or redundant comment.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Outside diff range and nitpick comments (1)
.github/workflows/preview.yml (1)

74-76: Remove temporary debug step 'TEMP LS'

Ohayo, sensei! The 'TEMP LS' step appears to be a temporary debugging step. If it's no longer needed, consider removing it to clean up the workflow.

Apply this diff to remove the step:

-          - name: TEMP LS
-            run: ls -R artifacts
Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0da6f0c and a18222b.

Files selected for processing (1)
  • .github/workflows/preview.yml (1 hunks)
Additional context used
actionlint
.github/workflows/preview.yml

22-22: the runner of "Swatinem/rust-cache@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


38-38: property "prepare" is not defined in object type {}

(expression)


39-39: shellcheck reported issue in this script: SC2086:info:2:61: Double quote to prevent globbing and word splitting

(shellcheck)


66-66: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


78-78: the runner of "docker/setup-buildx-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


81-81: the runner of "docker/login-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


89-89: shellcheck reported issue in this script: SC2086:info:2:51: Double quote to prevent globbing and word splitting

(shellcheck)

run: ls -R artifacts

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update docker/setup-buildx-action to the latest version

Ohayo, sensei! The docker/setup-buildx-action@v1 action is outdated. Updating to the latest version ensures improved functionality and support.

Apply this diff to update the action version:

-          - name: Set up Docker Buildx
-            uses: docker/setup-buildx-action@v1
+          - name: Set up Docker Buildx
+            uses: docker/setup-buildx-action@v2
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uses: docker/setup-buildx-action@v1
uses: docker/setup-buildx-action@v2
Tools
actionlint

78-78: the runner of "docker/setup-buildx-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


steps:
- name: Checkout repository
uses: actions/checkout@v2
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update actions/checkout action to the latest version

Ohayo, sensei! The actions/checkout@v2 action is outdated. Updating to the latest version will provide consistency and utilize the latest features.

Apply this diff to update the action version:

-          uses: actions/checkout@v2
+          uses: actions/checkout@v4
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uses: actions/checkout@v2
uses: actions/checkout@v4
Tools
actionlint

66-66: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

steps:
- uses: actions/checkout@v4

- uses: Swatinem/rust-cache@v1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update Swatinem/rust-cache action to a newer version

Ohayo, sensei! The Swatinem/rust-cache@v1 action is outdated. Updating to the latest version will ensure better compatibility and performance.

Apply this diff to update the action version:

-          - uses: Swatinem/rust-cache@v1
+          - uses: Swatinem/rust-cache@v2
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- uses: Swatinem/rust-cache@v1
- uses: Swatinem/rust-cache@v2
Tools
actionlint

22-22: the runner of "Swatinem/rust-cache@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

uses: docker/setup-buildx-action@v1

- name: Login to GitHub Container Registry
uses: docker/login-action@v1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update docker/login-action to the latest version

Ohayo, sensei! The docker/login-action@v1 action is outdated. Updating to the latest version will enhance security and compatibility.

Apply this diff to update the action version:

-          - name: Login to GitHub Container Registry
-            uses: docker/login-action@v1
+          - name: Login to GitHub Container Registry
+            uses: docker/login-action@v2
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uses: docker/login-action@v1
uses: docker/login-action@v2
Tools
actionlint

81-81: the runner of "docker/login-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

id: vars
run: |
git config --global --add safe.directory '*'
echo "sha_short=$(git rev-parse --short HEAD)" >> $GITHUB_OUTPUT
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ShellCheck SC2086: Quote variables to prevent globbing and word splitting

Ohayo, sensei! In your shell script, the variable $GITHUB_OUTPUT should be wrapped in double quotes to prevent word splitting and globbing issues.

Apply this diff to fix the issue:

-              echo "sha_short=$(git rev-parse --short HEAD)" >> $GITHUB_OUTPUT
+              echo "sha_short=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
echo "sha_short=$(git rev-parse --short HEAD)" >> $GITHUB_OUTPUT
echo "sha_short=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"

- name: Archive binaries
id: artifacts
env:
VERSION_NAME: v${{ needs.prepare.outputs.tag_name }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undefined 'prepare' in 'needs.prepare.outputs.tag_name'

Ohayo, sensei! The variable needs.prepare.outputs.tag_name refers to a job named prepare, which isn't defined in the workflow. This will cause the workflow to fail due to an undefined reference.

To fix this, either define a prepare job that outputs tag_name, or modify the variable to correctly reference an existing job's output. If tag_name is not needed, you can set a default value.

For example, set VERSION_NAME to a default value:

-              VERSION_NAME: v${{ needs.prepare.outputs.tag_name }}
+              VERSION_NAME: "v1.0.0"  # Replace with the appropriate version
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
VERSION_NAME: v${{ needs.prepare.outputs.tag_name }}
VERSION_NAME: "v1.0.0" # Replace with the appropriate version
Tools
actionlint

38-38: property "prepare" is not defined in object type {}

(expression)

VERSION_NAME: v${{ needs.prepare.outputs.tag_name }}
run: |
tar -czvf "dojo_${VERSION_NAME}_linux_amd64.tar.gz" -C ./target/release katana torii sozo
echo "file_name=dojo_${VERSION_NAME}_linux_amd64.tar.gz" >> $GITHUB_OUTPUT
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ShellCheck SC2086: Quote variables to prevent globbing and word splitting

Ohayo, sensei! In your shell script, the variable $GITHUB_OUTPUT should be wrapped in double quotes to prevent word splitting and globbing issues.

Apply this diff to fix the issue:

-              echo "file_name=dojo_${VERSION_NAME}_linux_amd64.tar.gz" >> $GITHUB_OUTPUT
+              echo "file_name=dojo_${VERSION_NAME}_linux_amd64.tar.gz" >> "$GITHUB_OUTPUT"
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
echo "file_name=dojo_${VERSION_NAME}_linux_amd64.tar.gz" >> $GITHUB_OUTPUT
echo "file_name=dojo_${VERSION_NAME}_linux_amd64.tar.gz" >> "$GITHUB_OUTPUT"

Copy link

codecov bot commented Sep 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.30%. Comparing base (21ec45e) to head (a09b21b).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2446      +/-   ##
==========================================
+ Coverage   68.21%   68.30%   +0.09%     
==========================================
  Files         365      365              
  Lines       48022    48034      +12     
==========================================
+ Hits        32757    32809      +52     
+ Misses      15265    15225      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Outside diff range and nitpick comments (1)
.github/workflows/preview.yml (1)

65-67: Consider removing the 'TEMP LS' debug step

Ohayo, sensei! The TEMP LS step seems to be a temporary debugging step. Consider removing it to keep the workflow clean and efficient.

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a18222b and ea45f9d.

Files selected for processing (1)
  • .github/workflows/preview.yml (1 hunks)
Additional comments not posted (2)
.github/workflows/preview.yml (2)

17-17: Verify the runner label 'ubuntu-latest-4-cores'

Ohayo, sensei! The runner label ubuntu-latest-4-cores is not a standard GitHub-hosted runner. Please verify that this label refers to a valid self-hosted runner in your environment or consider using runs-on: ubuntu-latest for GitHub-hosted runners.


88-88: Tagging strategy is properly implemented

Ohayo, sensei! The tagging strategy using the short SHA from steps.vars.outputs.sha_short is correctly implemented and will help in identifying the image versions.

Comment on lines 24 to 28
- uses: arduino/setup-protoc@v2
with:
repo-token: ${{ secrets.GITHUB_TOKEN }}
version: "25.x"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unnecessary 'repo-token' input from 'arduino/setup-protoc@v2'

Ohayo, sensei! The repo-token input is not required for the arduino/setup-protoc@v2 action. Removing it can prevent potential misconfigurations.

Apply this diff to remove the unnecessary input:

       - uses: arduino/setup-protoc@v2
         with:
-          repo-token: ${{ secrets.GITHUB_TOKEN }}
           version: "25.x"
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- uses: arduino/setup-protoc@v2
with:
repo-token: ${{ secrets.GITHUB_TOKEN }}
version: "25.x"
- uses: arduino/setup-protoc@v2
with:
version: "25.x"

.github/workflows/preview.yml Outdated Show resolved Hide resolved
.github/workflows/preview.yml Outdated Show resolved Hide resolved
.github/workflows/preview.yml Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Outside diff range and nitpick comments (5)
Dockerfile (2)

18-18: Consider adding SHA256 checksum verification for enhanced security.

To ensure the integrity of the downloaded Tini binary, it's a good idea to verify its SHA256 checksum.

You can modify the ADD instruction like this:

ENV TINI_VERSION v0.19.0
ENV TINI_SHA256 93dcc18adc78c65a028a84799ecf8ad40c936fdfc5f2a57b1acda5a8117fa82c

ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /tini
RUN echo "${TINI_SHA256} */tini" | sha256sum -c - \
    && chmod +x /tini

This downloads Tini, verifies its SHA256 checksum matches the expected value, and then makes it executable in a single RUN layer to keep the image lean.


Line range hint 1-16: Heads up sensei: The builder stage is missing Tini.

Looks like the builder stage, where the curtailer tool is built and installed, doesn't include Tini. If curtailer or its build process spawns child processes, you might run into zombie process issues in that stage.

Consider if the builder stage also needs Tini. If so, you can install it there similarly to the final stage, before running the build commands.

.github/Dockerfile (3)

1-1: Ohayo sensei! Consider using a more specific base image.

Using a specific version tag like rust:1.80.0-slim is great for reproducibility. However, consider using a more specific tag like rust:1.80.0-slim-buster to pin the underlying OS version as well. This will further improve reproducibility and avoid potential issues if the default OS version changes in the future.


13-14: Consider using a single RUN instruction for Rust setup.

To reduce the number of layers in the final image, consider combining the rustup commands into a single RUN instruction.

Here's how you can combine them:

RUN rustup install 1.79.0 && \
    rustup component add cargo clippy rust-docs rust-std rustc rustfmt

22-22: Consider using a more specific recipe path.

Using recipe.json as the recipe path is fine. However, consider using a more specific path like /app/recipe.json to avoid potential conflicts if there are other recipe.json files in the project.

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ea45f9d and 4afe768.

Files selected for processing (4)
  • .dockerignore (1 hunks)
  • .github/Dockerfile (1 hunks)
  • .github/workflows/preview.yml (1 hunks)
  • Dockerfile (1 hunks)
Files skipped from review due to trivial changes (1)
  • .dockerignore
Additional context used
actionlint
.github/workflows/preview.yml

44-44: shellcheck reported issue in this script: SC2046:warning:2:20: Quote this to prevent word splitting

(shellcheck)

Additional comments not posted (6)
Dockerfile (1)

17-20: Ohayo sensei! The Tini integration looks great.

The changes to add Tini as the init system for the container follow best practices. Key points:

  • Specifying the Tini version as an environment variable allows for easy updates in the future.
  • Downloading Tini from the official GitHub releases ensures authenticity.
  • Setting the executable permission on the Tini binary is necessary.
  • Using Tini as the entrypoint enables it to be the init process (PID 1).

Tini will help manage zombie processes and forward signals properly to the main process. Nice work!

.github/Dockerfile (4)

7-7: ****


10-11: Verify the LIBCLANG_PATH is correctly set.

Finding and exporting the LIBCLANG_PATH is a good practice. However, it's worth double-checking that the path is correctly set and accessible within the container.

You can verify it by adding an echo $LIBCLANG_PATH command after the export line and building the image to check the output.


16-16: LGTM!

Installing cargo-chef is a great way to optimize the build process and leverage caching.


29-30: Excellent use of cargo chef for caching dependencies!

Using cargo chef cook to build and cache dependencies is a great optimization technique. It significantly speeds up subsequent builds by leveraging the Docker layer caching mechanism.

.github/workflows/preview.yml (1)

1-57: LGTM!

Ohayo, sensei! The new GitHub Actions workflow looks great. It effectively automates the process of building and pushing Docker images for preview builds. The workflow is well-structured, utilizing caching and output variables to optimize the build process. The use of Docker Buildx and the GitHub Container Registry streamlines the CI/CD pipeline.

No major issues or concerns were identified during the review. The workflow should function as intended, enhancing the efficiency of the development workflow.

Tools
actionlint

44-44: shellcheck reported issue in this script: SC2046:warning:2:20: Quote this to prevent word splitting

(shellcheck)

.github/Dockerfile Outdated Show resolved Hide resolved
.github/Dockerfile Show resolved Hide resolved
.github/Dockerfile Show resolved Hide resolved
.github/workflows/preview.yml Outdated Show resolved Hide resolved
@steebchen steebchen changed the title Add GitHub Actions workflow for preview builds ci(workflows) add GitHub Actions workflow for preview builds Sep 19, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Outside diff range and nitpick comments (1)
.github/Dockerfile (1)

38-45: Sensei, the final stage can be simplified by using a single COPY instruction.

Instead of using multiple COPY instructions to copy the built binaries, consider using a single COPY instruction with wildcards to simplify the Dockerfile.

Here's how you can refactor it:

FROM rust:1-alpine

WORKDIR /

COPY --from=builder /app/target/release/{katana,torii,sozo,saya} /app/artifacts/

This will copy all the specified binaries in a single instruction, reducing the number of layers in the final image.

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4afe768 and 44e5efc.

Files selected for processing (4)
  • .dockerignore (1 hunks)
  • .github/Dockerfile (1 hunks)
  • .github/workflows/preview.yml (1 hunks)
  • .gitignore (1 hunks)
Files skipped from review due to trivial changes (1)
  • .gitignore
Files skipped from review as they are similar to previous changes (2)
  • .dockerignore
  • .github/workflows/preview.yml
Additional comments not posted (2)
.github/Dockerfile (2)

1-16: Ohayo sensei! The chef stage looks great!

The apt-get commands are combined to reduce the number of layers and the final image size. The apt cache is also cleaned up to further reduce the image size. The LIBCLANG_PATH environment variable is set correctly, and the Rust toolchain and components are installed correctly. The cargo-chef tool is also installed correctly.


18-22: The planner stage is set up correctly, sensei!

The planner stage is correctly based on the chef stage, and the working directory is set correctly. The application code is copied correctly, and the cargo chef prepare command is used correctly to generate the recipe file.

Comment on lines +24 to +36
FROM chef AS builder

WORKDIR /app
COPY --from=planner /app/recipe.json recipe.json
# Build dependencies - this is the caching Docker layer!
RUN cargo chef cook --release --recipe-path recipe.json
RUN cargo build --release --bins # pre-cache some stuff

# Build application
COPY . .
ENV PATH="/root/.cargo/bin:${PATH}"

RUN cargo build --release --bins
Copy link

@coderabbitai coderabbitai bot Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sensei, there are a few things that can be improved in the builder stage.

The pre-caching step on line 30 is redundant since the same command is already executed on line 36. Consider removing the redundant command to simplify the Dockerfile and reduce build time.

Also, copying the application code again on line 33 is unnecessary since it was already copied in the planner stage. Setting the PATH environment variable on line 34 is also unnecessary since it is not used in the final image.

Here's how you can refactor it:

FROM chef AS builder

WORKDIR /app
COPY --from=planner /app/recipe.json recipe.json
# Build dependencies - this is the caching Docker layer!
RUN cargo chef cook --release --recipe-path recipe.json

RUN cargo build --release --bins

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still caches building the deps, so it certainly speeds up the build when dependencies don't change

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steebchen Ohayo, sensei! Thank you for the clarification. I understand now that the pre-caching step helps speed up the build when dependencies don't change. I'll keep this in mind for future reviews.

(・ェ・)


Learnings added
Learnt from: steebchen
PR: dojoengine/dojo#2446
File: .github/Dockerfile:24-36
Timestamp: 2024-09-19T19:02:02.095Z
Learning: In the Dockerfile, the pre-caching step in the `builder` stage is important for caching dependencies and speeding up the build when dependencies don't change.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (2)
.github/workflows/preview.yml (2)

26-33: Consider consolidating the caching steps.

Ohayo, sensei! I noticed that there are two separate caching steps for Docker layers, one for /tmp/.buildx-cache/prebuild and another for /tmp/.buildx-cache/build. Having two steps for the same purpose can be confusing and unnecessary. Consider consolidating them into a single caching step for clarity and simplicity.


81-89: Temporary fix acknowledged.

Ohayo, sensei! The temporary fix for moving the Docker build cache is noted. It's good that the referenced GitHub issues are mentioned for context. While this fix is necessary for now, it's important to keep tracking those issues for a permanent solution. Consider adding a TODO comment to revisit this once the underlying issues are resolved.

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 55819aa and ba0273b.

Files selected for processing (1)
  • .github/workflows/preview.yml (1 hunks)
Additional context used
actionlint
.github/workflows/preview.yml

11-11: label "ubuntu-latest-32-cores" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

Additional comments not posted (9)
.github/workflows/preview.yml (9)

11-11: Verify the runner label.

Ohayo, sensei! The runner label ubuntu-latest-32-cores is not a standard GitHub-hosted runner label. If it's a custom label for a self-hosted runner, please ensure it's defined in the actionlint.yaml config file to avoid validation errors.

Tools
actionlint

11-11: label "ubuntu-latest-32-cores" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)


14-15: LGTM!

The usage of docker/setup-buildx-action@v3 looks good. It will set up Docker Buildx with the latest version of the action.


17-24: LGTM!

The caching configuration for Docker layers looks good. It will help speed up subsequent builds by caching the layers specific to the branch and commit. The fallback restore keys are also properly configured.


35-36: LGTM!

Using actions/checkout@v4 to check out the repository is the right approach. It ensures the latest version of the action is used.


38-43: LGTM!

The configuration for logging in to the GitHub Container Registry looks good. Using docker/login-action@v3 with the appropriate registry, username, and password ensures secure authentication for pushing Docker images.


45-50: LGTM!

Setting the safe.directory Git configuration and the sha_short output variable is properly implemented. Using ${{ github.workspace }} and $GITHUB_OUTPUT ensures the correct values are set.


52-61: LGTM!

The configuration for building the Docker image using docker/build-push-action@v5 looks good. The build context, Dockerfile path, load option, image tagging, cache options, and target platform are all properly set up.


63-68: LGTM!

Building the local binaries using a Docker container is a good approach. Mounting the artifacts directory and copying the built artifacts to the mounted volume ensures they are accessible on the host. Setting the PLATFORM environment variable to linux/amd64 is consistent with the target platform.


70-79: LGTM!

The configuration for building and pushing the Docker image using docker/build-push-action@v3 looks good. The push option, image tagging, cache options, target platform, and build contexts are all properly set up. Including the artifacts build context ensures the previously built artifacts are included in the image.

.github/workflows/preview.yml Outdated Show resolved Hide resolved
@glihm glihm merged commit 8b7d6b2 into main Sep 20, 2024
15 checks passed
@glihm glihm deleted the ci-preview branch September 20, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants